Add Fi* objects and change some others. - #13
Conversation
|
Thank you for contributing your definitions! That's exactly what this repository needs. However, in order to add this upstream, some clarification / changes are needed. Please note I have not had access to an AFAS instance for the past ~7 years, so I can't test things myself. One thing about the FI* objects:
So why not just add the FI* definitions into UpdateObject? (There's nothing wrong with adding them as new classes, but it makes the code inconsistent, and I fear things will get harder to find then.) I might still have merged this PR if that was the only thing, but there are more significant things - which I will post as review comments near the code. |
| 'type' => 'blob', | ||
| ], | ||
| ], | ||
| ]; |
There was a problem hiding this comment.
I assume you've seen the KnSubject definitions in UpdateObject. These are now not used anymore, so at the very least they should be cleaned up. Or: the definitions should be changed in there. (I don't know what is better, in this KnSubject case.)
Please clarify (mag ook in het Nederlands als je wilt) why you have done it this way, because the biggest change I see is that you have effectively deleted a lot of field definitions.
I see you've added `'type' => 'blob' to FileStream. That's fine. Also adding extra aliases is fine.
But if you are adding 'required', please specify that this is actually required globally in AFAS, not just for your instance.
Note: I may not have good enough knowledge of AFAS and the definitions I provided for KnSubject may be non-general == not applicable for this upstream repo. But if so: please tell me that.
Also, re. the aliases:
- they are currently all 'English based', only lowercase + underscores.
- This PR adds a mix of English + Dutch, with upper + lowercase + spaces.
If you want to have aliases like these in your local system: great. There are ways to override the aliases. But filing this as a PR upstream... just makes thing inconsistent.
| // Default of true is set dynamically in | ||
| // validateFields(), depending on presence of | ||
| // KnAddress objects. | ||
| ], |
There was a problem hiding this comment.
First of all: this should be moved down to around line 638 so it's better understandable what we are talking about here.
Then: there is already a field with alias postal_address_is_address. We are not going to have two fields with the same alias committed to this repository.
Your change looks very logical. But, as you can probably tell from line ~638: 'PadAdr' did not work for me in KnOrganisation object; this was called 'PbAd' for me.
- Did AFAS change this in the past ~9 years while I wasn't looking? (Did AFAS document this anywhere?) Or was this a weird thing on my side?
- Does 'PbAd' still exist, or is that now gone? (If it's gone: we should remove the 'PbAd' definition.)
(If this + the VaTP additions were the only changes in this PR, I would not wait to add it... but the 'PbAd' alias needs to be removed at the same time.)
| // TypeRekening | ||
| 'VaTp' => [ | ||
| 'alias' => 'type_verkooprelatie', | ||
| ], |
There was a problem hiding this comment.
Just checking, want Rekening != Verkooprelatie in mijn hoofd:
- Is "TypeRekening" een omschrijving in AFAS docs? (Ik heb ook letterlijke AFAS omschrijvingen in de code geplempt, dus dat is best, het is beter dan niks als documentatie
- Maar was is het echt? Verwijst het naar een rekening/account of naar een verkooprelatie?
- Heeft dit "type" veld slechts een aantal toegestane waarden? Zijn dat nummers, of IDs van een ander type AFAS object?
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "wyz/practicalafas", | |||
| "name": "kjsoftware/practicalafas", | |||
There was a problem hiding this comment.
I understand you need your own branch to refer to in your own build system or whatever. And you're solving internal tickets in that branch.
But don't put this in a PR that you submit to upstream. Create another branch without this change.
(Otherwise I might still add selected code changes... but copypasting them manually and closing this PR.)
3 participants
No description provided.